Skip to content

feat: allow specifying the chat template as base64 to avoid weird escaping and templating issues#534

Merged
dushyantbehl merged 1 commit into
foundation-model-stack:mainfrom
HarikrishnanBalagopal:feat/chattemplatebase64
Apr 28, 2025
Merged

feat: allow specifying the chat template as base64 to avoid weird escaping and templating issues#534
dushyantbehl merged 1 commit into
foundation-model-stack:mainfrom
HarikrishnanBalagopal:feat/chattemplatebase64

Conversation

@HarikrishnanBalagopal
Copy link
Copy Markdown
Contributor

@HarikrishnanBalagopal HarikrishnanBalagopal commented Apr 19, 2025

Description of the change

Related issue number

How to verify the PR

Was the PR tested

  • I have added >=1 unit test(s) for every new method I have added.
  • I have ensured all unit tests pass

@github-actions
Copy link
Copy Markdown

Thanks for making a pull request! 😃
One of the maintainers will review and advise on the next steps.

Copy link
Copy Markdown
Collaborator

@dushyantbehl dushyantbehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HarikrishnanBalagopal Please add a unit test for the base64 chat template and also say somewhere that this is a undocumented feature...i.e. in the schema you should try to add chat_template_base64 but add a note that this is something users should use if they know what they are doing.

Any way to check if the base64 decode happened correctly? or throw some error if it didn't result in a chat template?

assert isinstance(
chat_template_base64, str
), "chat_template_base64 should be a string"
logger.warning(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also look to print the chat tempalte along side the warning...what was decoded one so users know what was picked up.

Copy link
Copy Markdown
Contributor Author

@HarikrishnanBalagopal HarikrishnanBalagopal Apr 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...what was decoded one so users know what was picked up.

ok but if it's not a string then we can't decode it right?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add that post the warning that you have picked this chat template at that place it will be a str due to assertion

Comment thread tuning/data/data_config.py Outdated
@HarikrishnanBalagopal
Copy link
Copy Markdown
Contributor Author

HarikrishnanBalagopal commented Apr 27, 2025

and also say somewhere that this is a undocumented feature...i.e. in the schema you should try to add chat_template_base64 but add a note that this is something users should use if they know what they are doing.

Then it would become documented, wouldn't it?

Any way to check if the base64 decode happened correctly? or throw some error if it didn't result in a chat template?

The b64decode function will throw an Exception if it's an invalid base64 string. Example:

>>> from base64 import b64decode
>>> s = 'asdadasda'
>>> b64decode(s)
Traceback (most recent call last):
  File "<python-input-2>", line 1, in <module>
    b64decode(s)
    ~~~~~~~~~^^^
  File "/opt/homebrew/Cellar/python@3.13/3.13.1/Frameworks/Python.framework/Versions/3.13/lib/python3.13/base64.py", line 88, in b64decode
    return binascii.a2b_base64(s, strict_mode=validate)
           ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^
binascii.Error: Invalid base64-encoded string: number of data characters (9) cannot be 1 more than a multiple of 4

The .decode("utf-8") function will also throw an Exception if it's not valid.

@HarikrishnanBalagopal
Copy link
Copy Markdown
Contributor Author

@HarikrishnanBalagopal Please add a unit test for the base64 chat template and also say somewhere that this is a undocumented feature...i.e. in the schema you should try to add chat_template_base64 but add a note that this is something users should use if they know what they are doing.

Any way to check if the base64 decode happened correctly? or throw some error if it didn't result in a chat template?

I have added tests

Copy link
Copy Markdown
Collaborator

@dushyantbehl dushyantbehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes requested

)
DATA_CONFIG_MULTITURN_CHAT_TOKENIZE_AND_MASKING_DATA_BASE64_HANDLER_EXPECTED_CHAT_TEMPLATE = os.path.join(
PREDEFINED_DATA_CONFIGS,
"mt_data_granite_3_1B_tokenize_and_mask_base64_handler_expected_chat_template.txt",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this file also to
granite_3_1b_chat_template.txt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread tests/artifacts/predefined_data_configs/__init__.py Outdated
Comment thread tests/artifacts/predefined_data_configs/__init__.py Outdated
Comment thread tests/artifacts/predefined_data_configs/__init__.py Outdated
Comment thread tests/artifacts/predefined_data_configs/__init__.py Outdated
Comment thread tests/artifacts/predefined_data_configs/__init__.py Outdated
assert isinstance(
chat_template_base64, str
), "chat_template_base64 should be a string"
logger.warning(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add that post the warning that you have picked this chat template at that place it will be a str due to assertion

Comment thread tuning/data/data_config.py Outdated
@dushyantbehl
Copy link
Copy Markdown
Collaborator

@HarikrishnanBalagopal requested minor name changes and post them please also fix the format error

@HarikrishnanBalagopal HarikrishnanBalagopal force-pushed the feat/chattemplatebase64 branch 3 times, most recently from 21777b4 to 8075828 Compare April 28, 2025 10:45
@HarikrishnanBalagopal
Copy link
Copy Markdown
Contributor Author

@HarikrishnanBalagopal requested minor name changes and post them please also fix the format error

done

…aping and templating issues

test: create tests for chat_template_base64 field

Signed-off-by: Harikrishnan Balagopal <harikrishmenon@gmail.com>
Copy link
Copy Markdown
Collaborator

@dushyantbehl dushyantbehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dushyantbehl dushyantbehl merged commit 7ba3481 into foundation-model-stack:main Apr 28, 2025
9 checks passed
dushyantbehl pushed a commit to dushyantbehl/fms-hf-tuning that referenced this pull request Jun 23, 2025
…aping and templating issues (foundation-model-stack#534)

test: create tests for chat_template_base64 field

Signed-off-by: Harikrishnan Balagopal <harikrishmenon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants